Skip to content

fix(1439): kick-only flow no longer tells the player they were banned#1454

Merged
rumblefrog merged 1 commit into
mainfrom
fix/1439-kickit-banned-message-on-kick
May 25, 2026
Merged

fix(1439): kick-only flow no longer tells the player they were banned#1454
rumblefrog merged 1 commit into
mainfrom
fix/1439-kickit-banned-message-on-kick

Conversation

@rumblefrog

Copy link
Copy Markdown
Member

Summary

  • Kicking payer displays message that they have been banned #1439 root cause: the kickit iframe (loaded by the right-click → "Kick player" context menu on ?p=servers) ran the same code path as the post-ban iframe embed inside admin.bans.php's "Ban Added" success dialog — emitting the rcon kick reason "You have been banned by this server, check $domain for more info" and silently mutating any active ban that shared the SteamID (UPDATE :prefix_bans SET sid = :sid WHERE authid = :authid AND RemovedBy IS NULL). The reporter saw the wrong message; the silent audit-trail corruption was the worse half of the bug.
  • Fix: carry an explicit mode signal end-to-end from the context-menu URL through the iframe template down into api_kickit_kick_player. Kick mode skips the ban UPDATE entirely, emits a kick-appropriate rcon reason, and lands the operator back on ?p=servers after the 5s read-the-result settle (the ban mode keeps its existing ?p=admin&c=bans destination). Pre-Kicking payer displays message that they have been banned #1439 callers that don't supply the param coerce to 'ban' so the post-ban iframe embed in admin.bans.php keeps working untouched.
  • Coverage: 15-case PHPUnit handler-shape suite (mode round-trip, unknown-mode coercion, rcon-message branching, ban-UPDATE gate via a new _api_kickit_should_update_ban_sid helper); 2 new E2E specs — kickit-iframe.spec.ts drives the full kick flow end-to-end (title says "Kick player", payload carries mode: 'kick', real waitForURL against the actual 5s redirect proves the kick-mode redirect arm executed) and server-player-context-menu.spec.ts pins &mode=kick in the Kick item's href.
  • Adversarial reviewer pass spawned mid-PR caught: (1) AGENTS.md SQL example was incorrectly transcribed; (2) the ban-UPDATE gate was tested only via the rcon-message side-channel — extracted into a dedicated helper with paired unit tests + a static-analysis guard that the handler invokes the helper rather than inlining $mode === 'ban'; (3) the previous E2E redirect assertion grepped the script source (which carries both literals via a ternary regardless of mode) — replaced with page.waitForURL on the real browser navigation; (4) KickitView ctor reordered to put the new $mode last with an inline 'ban' default so the param-order surface is back-compat-friendly; (5) tests now also assert mode is consumed by the handler and not echoed back in the API response.

Test plan

  • ./sbpp.sh phpstan (level 5 + dba) — green
  • ./sbpp.sh test (898 tests, 3379 assertions) — green
  • ./sbpp.sh ts-check — green
  • ./sbpp.sh composer api-contract — regenerated docblock for ApiKickitKickPlayerRequest, committed
  • CI=1 ./sbpp.sh e2e --project chromium (219 passed, 100 mobile-skipped) — green
  • Manual: right-click a player on ?p=servers → Kick → iframe title reads "Kick player", post-kick redirect lands back on ?p=servers
  • Manual: ban a player on ?p=admin&c=bans (existing flow) → post-ban iframe still embeds, still hits the :prefix_bans UPDATE, still sends "You have been banned by this server, check ..." rcon reason, redirects to the bans admin page

Fixes #1439.

Right-clicking a player on `?p=servers` and choosing "Kick player" loads
the kickit iframe, which until now ran the post-ban-completion code path
unconditionally — emitting the rcon kick reason
"You have been banned by this server, check $domain for more info" and
re-attributing whatever active ban happened to share the SteamID to the
kick target's server (UPDATE :prefix_bans SET sid = ... WHERE authid = ...
AND RemovedBy IS NULL). The reporter on #1439 saw the wrong message; the
silent audit-trail corruption was the worse half of the bug.

The fix carries an explicit `mode` signal end-to-end:

- The context menu's Kick URL now appends `&mode=kick` (the post-ban
  iframe embed inside `admin.bans.php`'s "Ban Added" dialog stays on the
  default `'ban'` mode).
- `admin.kickit.php` allowlists `$_GET['mode']` to `'ban'|'kick'`
  (anything else coerces to `'ban'` — backward-compat with pre-#1439
  callers that don't supply the param).
- `KickitView` carries the mode through to `page_kickit.tpl` which
  branches the `<title>` ("Kick player" vs "Ban player"), surfaces a
  `data-mode` attribute on the container for third-party theme styling,
  and forwards the value as `mode` on every `kickit.kick_player` JSON
  call.
- `api_kickit_kick_player` re-validates the mode and gates two things on
  it: the `:prefix_bans` UPDATE is skipped on kick mode (extracted into
  `_api_kickit_should_update_ban_sid` for testability + clarity), and
  the rcon kick reason is now "You have been kicked from this server"
  on kick mode (still "You have been banned by this server, check ..."
  on ban mode).
- The iframe's post-completion redirect lands the operator back on
  `?p=servers` for the kick flow (where they came from), preserving the
  existing `?p=admin&c=bans` destination for ban mode.

Coverage:

- `KickitTest` (15 cases, 82 assertions) — handler-shape coverage for
  both modes + the unknown-mode coercion, the rcon-message branch via
  `_api_kickit_build_kick_message`, and the ban-UPDATE gate via the new
  `_api_kickit_should_update_ban_sid` helper (incl. a static-analysis
  guard that the handler invokes the helper rather than inlining the
  `$mode === 'ban'` check around the UPDATE).
- `kickit-iframe.spec.ts` adds an E2E case driving the full kick flow:
  title says "Kick player", `kickit.kick_player` payload carries
  `mode: 'kick'`, and the post-completion `window.location` flip lands
  on `index.php?p=servers` (anchored on real `waitForURL` against the
  actual 5s redirect timer — source-grep on the script body would
  silently accept a regression that deleted the kick-mode arm).
- `server-player-context-menu.spec.ts` updated to expect `&mode=kick`
  in the Kick item's href.

Documentation in AGENTS.md (context-menu prose + "Where to find what"
table + regression-guards section) extended to spell the `mode`
contract end-to-end so future readers don't re-discover the failure
mode.

Adversarial reviewer pass also caught: ban-mode UPDATE SQL was
incorrectly transcribed as `removetype = 'X'` in AGENTS.md (now
`sid = :sid WHERE authid = :authid AND RemovedBy IS NULL`); the new
unit tests now also assert the `mode` field is consumed by the handler
and not echoed back in the API response.

Fixes: #1439
@rumblefrog
rumblefrog force-pushed the fix/1439-kickit-banned-message-on-kick branch from c2ab879 to 4dc6cb0 Compare May 25, 2026 19:36
@rumblefrog
rumblefrog added this pull request to the merge queue May 25, 2026
Merged via the queue into main with commit fafebc1 May 25, 2026
8 checks passed
@rumblefrog
rumblefrog deleted the fix/1439-kickit-banned-message-on-kick branch May 25, 2026 19:45
@github-actions github-actions Bot locked and limited conversation to collaborators May 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kicking payer displays message that they have been banned

1 participant